[rollout, diffusion] fix: support prompt embedding cache for Qwen-Image#214
[rollout, diffusion] fix: support prompt embedding cache for Qwen-Image#214Sky-Trigger wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a prompt embedding cache mechanism for diffusion workers by adding enable_prompt_embed_cache and prompt_embed_cache_size configurations across trainer and rollout configs. It also refactors several rollout adapters (such as Qwen and Wan22) to support list-based inputs for prompt IDs and masks, standardizing batch size calculation via a new helper function get_prompt_batch_size. There are no review comments provided, so I have no feedback to provide.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
@knlnguyen1802 , prompt cache related, PTAL |
|
@Sky-Trigger Could you help to run a verification and post a curve |
|
@Sky-Trigger Thanks for verify it, could you run several step to compare performance gain on this branch ? |
|
I conducted tests using |
| prompt_ids: torch.Tensor, | ||
| prompt_mask: torch.Tensor | None, | ||
| negative_prompt_ids: torch.Tensor | None, | ||
| negative_prompt_mask: torch.Tensor | None, |
There was a problem hiding this comment.
I think should recover all change like this, we should focus on feature, the fix in type should do in a separate PR if necessary
| if isinstance(prompt_ids, list): | ||
| prompt_ids = torch.tensor(prompt_ids, device=self.device) | ||
| if isinstance(negative_prompt_ids, list): | ||
| negative_prompt_ids = torch.tensor(negative_prompt_ids, device=self.device) |
There was a problem hiding this comment.
Recover these change also
|
@Sky-Trigger Thank you for the PR, we should focus only on the feature change mention in this PR, every type correction and code linting, code refactor need to recover. We can fix them in a different PR if necessary |
agree, pls focus on the feature only, thanks |
|
Thanks for the review. I may have misunderstood what you mean by “recover,” so I would like to clarify the expected implementation before making further changes. My understanding of the review comments is that I should:
if isinstance(prompt_ids, list):
prompt_ids = torch.tensor(prompt_ids, device=self.device)
if isinstance(negative_prompt_ids, list):
negative_prompt_ids = torch.tensor(
negative_prompt_ids,
device=self.device,
)However, I am not sure how to reconcile the second change with the behavior introduced by vLLM-Omni PR #2962. In PR #2962, the prompt embedding cache builds its key from the arguments passed to The original verl-omni path is: The purpose of this PR is to change it to: Therefore, if I restore the conversion in The type annotation changes were also made for this same reason. After delaying the tensor conversion, Could you please clarify which implementation you would prefer?
I can also revert the shared batch-size helper and any unrelated formatting or refactoring changes. I mainly need clarification on whether the delayed list-to-tensor conversion itself should remain, since it appears necessary for compatibility with PR #2962. |
I think this PR do some unnecessary refactor and linting, let's me give a more detail review on these point |
|
Not every model is in our CI, we should minimise the code change as possible to prevent any unexpected code break, thx. |
|
I’ll revert the unnecessary redundant changes and keep only the minimal compatibility changes required to enable prompt caching for the Qwen-Image model. |
knlnguyen1802
left a comment
There was a problem hiding this comment.
@Sky-Trigger Please revert these thing and similar thing
| prompt_ids: torch.Tensor | list[int] | list[list[int]], | ||
| attention_mask: torch.Tensor | list[int] | list[list[int]] | None = None, |
| from verl_omni.pipelines.qwen_image_flow_grpo.common import ( | ||
| build_img_shapes, | ||
| coalesce_not_none, | ||
| get_prompt_batch_size, | ||
| ) |
There was a problem hiding this comment.
All change in this file is unrelated to embedding cache, it's just convert prompt_ids to list then convert it back to tensor ?
There was a problem hiding this comment.
It is true that the input is first converted to a list and later converted back to a tensor. However, the timing of these conversions is important.
After prompt_ids is converted to a list, the encode_prompt() wrapper introduced in PR #2962 can automatically apply the prompt embedding cache. The wrapper cannot cache calls when prompt_ids is a Tensor.
Therefore, the purpose of this PR is to delay the tensor conversion of prompt_ids and the other related inputs until after they have passed through the encode_prompt() wrapper, so that the prompt cache provided by PR #2962 can take effect.
| if isinstance(prompt_ids, list): | ||
| prompt_ids = torch.tensor(prompt_ids, device=self.device) | ||
| if isinstance(negative_prompt_ids, list): | ||
| negative_prompt_ids = torch.tensor(negative_prompt_ids, device=self.device) |
There was a problem hiding this comment.
Do you mean that this conversion should be controlled by whether the prompt embedding cache is enabled?
For example:
if not prompt_embed_cache_enabled:
if isinstance(prompt_ids, list):
prompt_ids = torch.tensor(prompt_ids, device=self.device)
if isinstance(negative_prompt_ids, list):
negative_prompt_ids = torch.tensor(
negative_prompt_ids,
device=self.device,
)When the prompt embedding cache is enabled, the token IDs would remain as lists until they pass through the cached encode_prompt() call. When the cache is disabled, we would preserve the original behavior and convert them to tensors at this point.
Is this the implementation you are suggesting?
There was a problem hiding this comment.
I will revise the implementation to preserve maximum compatibility with the existing behavior and add only the code strictly required when the prompt cache is enabled.
| prompt_mask: torch.Tensor | None = None, | ||
| prompt_mask: torch.Tensor | list[int] | list[list[int]] | None = None, | ||
| negative_prompt_ids: torch.Tensor | list[int] | None = None, | ||
| negative_prompt_mask: torch.Tensor | None = None, | ||
| negative_prompt_mask: torch.Tensor | list[int] | list[list[int]] | None = None, |
| prompt_ids: torch.Tensor, | ||
| attention_mask: torch.Tensor | None = None, | ||
| prompt_ids: torch.Tensor | list[int] | list[list[int]], | ||
| attention_mask: torch.Tensor | list[int] | list[list[int]] | None = None, |
| if isinstance(prompt_ids, list): | ||
| prompt_ids = torch.tensor(prompt_ids, device=self.device) | ||
| if isinstance(attention_mask, list): | ||
| attention_mask = torch.tensor(attention_mask, device=self.device) |
There was a problem hiding this comment.
If you want to refactor this as common, then can do it in a separate PR
| prompt_mask: torch.Tensor | None = None, | ||
| prompt_mask: torch.Tensor | list[int] | list[list[int]] | None = None, | ||
| negative_prompt_ids: torch.Tensor | list[int] | None = None, | ||
| negative_prompt_mask: torch.Tensor | None = None, | ||
| negative_prompt_mask: torch.Tensor | list[int] | list[list[int]] | None = None, |
| prompt_ids: torch.Tensor, | ||
| attention_mask: torch.Tensor | None = None, | ||
| prompt_ids: torch.Tensor | list[int] | list[list[int]], | ||
| attention_mask: torch.Tensor | list[int] | list[list[int]] | None = None, |
| prompt_mask: torch.Tensor | None = None, | ||
| prompt_mask: torch.Tensor | list[int] | list[list[int]] | None = None, | ||
| negative_prompt_ids: torch.Tensor | list[int] | None = None, | ||
| negative_prompt_mask: torch.Tensor | None = None, | ||
| negative_prompt_mask: torch.Tensor | list[int] | list[list[int]] | None = None, |
| def get_prompt_batch_size(prompt_ids: torch.Tensor | list[int] | list[list[int]]) -> int: | ||
| if isinstance(prompt_ids, torch.Tensor): | ||
| return prompt_ids.shape[0] if prompt_ids.ndim == 2 else 1 | ||
| return len(prompt_ids) if prompt_ids and isinstance(prompt_ids[0], list) else 1 |
There was a problem hiding this comment.
I think it's a good idea to refactor this as common feature, but unrelated to this PR
Please do it in a separate PR, and revert all related things
67154d5 to
f7cf431
Compare
|
I have completed the changes. Please review them again. @knlnguyen1802 @zhtmike |
|
I still believe that the changes introduced in my first version are necessary, specifically the changes to the accepted input types and the decision to defer the tensor conversion of I think there may have been some misunderstanding regarding my previous explanation. The changes described above are required for prompt caching to function correctly. Simply adding configuration options such as In addition, these changes are mainly made in the adapter layer and |
You can do 2 separate PR for them, refactor PR usually easy to review. Because our CI does not cover too broad yet so decouple PR is prefered |
|
Please take another look at the latest changes. This version now contains only the minimum modifications necessary to enable the required support only for Qwen-Image. |
Signed-off-by: Trigger <129651635+Sky-Trigger@users.noreply.github.com>
I think we can do it for Qwen-Image first, to see any regression, later can expand for another model |
|
Overall LGTM, cc @zhtmike for final review |
|
@wtomin and @AndyZhou952 , for the DPO / DiffusionNFT change, PTAL |
| custom_prompt: OmniCustomPrompt = {"prompt_token_ids": prompt_ids} | ||
| if prompt_mask is not None: | ||
| custom_prompt["prompt_mask"] = prompt_mask | ||
| custom_prompt["prompt_mask"] = ( |
There was a problem hiding this comment.
If unconditionally converting prompt_mask from tensor to list, will it cause some errors when calling attention_mask.ndim? Please double check it. A better solution might be converting prompt_mask to list only wen prompt embedding is enabled.
|
After an offline discussion with @knlnguyen1802, we agreed that it may create too much development burden if we are forced to convert tensors to lists just to enable the prompt cache. It may be better to fix this on the vLLM‑Omni side instead. Let us hold for this PR now |
|
ok tks,pls let me know if any further changes are needed. |


Keep token IDs and attention masks hashable until
encode_prompt(), convert them to tensors inside token-ID encoders, and expose vLLM-Omni prompt embedding cache controls through the diffusion rollout configuration.What does this PR do?
This PR adapts verl-omni's tokenized diffusion prompt path to the prompt embedding cache introduced by vLLM-Omni PR #2962.
Previously, rollout adapters converted token ID lists and masks to
torch.Tensorbefore callingencode_prompt(). Since the vLLM-Omni cache cannot build keys from tensor arguments, these calls bypassed the cache.This change keeps prompt IDs and masks list-based across the cache boundary and converts them to tensors only inside the underlying token-ID encoder. It covers:
It also adds rollout configuration for enabling and sizing the cache without requiring environment variables.
Checklist Before Starting
[rollout, diffusion] fix: support prompt embedding cache for tokenized promptsTest
Static checks completed:
ruff checkruff format --checkcompileallgit diff --checkManual validation:
examples/diffusionnft_trainer/run_qwen_image_ocr_lora.shcompleted successfully with the updated DiffusionNFT adapter.Configuration unit tests were added to
tests/workers/config/test_diffusion_config_on_cpu.py.Full pytest and all adapter-specific GPU/NPU experiments were not run in the current environment. The remaining independent rollout paths are FlowGRPO, online DPO, and Wan2.2 DanceGRPO.
API and Usage Example
The cache is disabled by default and uses the upstream default capacity of 32.
It can be enabled through Hydra configuration:
python3 -m verl_omni.trainer.main_diffusion \ actor_rollout_ref.rollout.enable_prompt_embed_cache=True \ actor_rollout_ref.rollout.prompt_embed_cache_size=64 \ ...Equivalent YAML:
No example shell scripts are modified.
Design & Code Changes
Prompt cache boundary
Before:
After:
Adapter changes
encode_prompt().prompt_maskto a list before constructing the diffusion request.Configuration changes
Added to
DiffusionRolloutConfig:These values are passed directly to
AsyncOmni. Cache size must be a positive integer.Checklist Before Submitting
pre-commit install && pre-commit run --all-files --show-diff-on-failure --color=alwaysAI Assistance
AI assistance was used to inspect the adapter call paths, implement the changes, and prepare this PR description. All changed lines and validation results were reviewed by the human submitter.